feat(scheduler): add gpujoborder plugin for configurable JobOrderFn tiebreak - #1995
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CoolingCube Can you please use the PR template ? https://github.com/kai-scheduler/KAI-Scheduler/blob/main/.github/pull_request_template.md |
CoolingCube
changed the title
…reak Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
…lause Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
d6b6be8 to
c5ff98c
Compare
…larity Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
4170bca to
279043f
Compare
CoolingCube
changed the title
Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
📊 Performance Benchmark ResultsComparing PR (
|
…hmark regression Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
|
The benchmark bot's regression above was real, traced it to the reclaim action rebuilding its entire victims queue once per candidate reclaimer job (pre-existing behavior), which made |
Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
|
Total coverage: 54.6% -> 54.6% (delta 0.00%) Merging this branch changes the coverage (1 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
Just checking in, this has been green for a while now (perf fix, license/formatting fix, and the naming/constants changes from your review are all in). No rush at all, just didn't want it to slip off the radar. Let me know if there's anything else you'd like changed. |
enoodle
left a comment
enoodle
left a comment
There was a problem hiding this comment.
Thanks @CoolingCube ! I wanted to look for ways to allow integrating such plugins more easily without requiring review and merge into this repo, but I think we should merge it for now anyway.
|
Hi @CoolingCube, thanks for contributing. |
|
Good question @gshaibi and worth being precise: this isn't hypothetical. |
Thanks, I agree that allocation order is a separate question. Since this change is intended to control victim selection, it shouldn’t affect the order of pending jobs. Also, because the victim queue reverses Would it be possible to scope this behavior to victim selection only and add a test confirming that pending-job ordering remains unchanged? |
Ran it against the merged code directly and it matches exactly what you flagged: |
…ctimOrderFn extension point Fixes a sign-inversion bug where prefer-larger evicted the smaller job (the victim queue's !JobOrderFn inversion flipped the same sign used for pending-job ordering). Adds Session.VictimOrderFn/AddVictimOrderFn as a dedicated, non-inverted composition path for victim-specific comparators, falling back to the existing !JobOrderFn behavior when none are registered, so no other plugin's behavior changes. gpujoborder now registers exclusively via AddVictimOrderFn and no longer affects pending-job allocation ordering at all. Addresses @gshaibi's review comment on kai-scheduler#1995. Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
|
Pushed the fix added a dedicated |
gshaibi
left a comment
gshaibi
left a comment
There was a problem hiding this comment.
Looks great, left two more comments.
…erFns; rename modes for clarity Addresses @gshaibi's two review comments: 1. VictimOrderFn now checks the JobOrderFns chain first (priority.go, elastic.go, etc.) and only falls through to VictimOrderFns as a tiebreak when no registered JobOrderFn has an opinion. Previously, VictimOrderFns were checked unconditionally first, letting a raw resource-size comparator outrank elastic's at-min/above-min protection. New test confirms elastic correctly outranks gpujoborder now. 2. Renamed prefer-larger/prefer-smaller to evict-larger-first/ evict-smaller-first to remove ambiguity about whether the mode means 'prefer to keep' or 'prefer to evict' the larger job. Added package and constant documentation. Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
…helper Each task in a multi-task subgroup was getting an identical UID, causing the second task to silently overwrite the first in the pod map used by GetAliveTasksRequestedGPUs(). Didn't affect the test's pass/fail correctness (elastic's own task-count tracking uses a separate counter, unaffected by the collision, and short-circuits before gpujoborder's comparator is ever consulted) but did produce a misleading debug log value. Fixed by giving each task a unique UID. Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
|
Pushed a fix for both For the composition order: you're right, For the naming: agreed, renamed to |
Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
davidLif
left a comment
davidLif
left a comment
There was a problem hiding this comment.
With the addition of VictimOrderFn, this PR needs to add some integration tests making sure that plugins with a JobOrderFn still effect the victims order, preserving exisitng logic.
… convention Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
Signed-off-by: CoolingCube <CoolingCubeInfo@proton.me>
|
@davidLif, I went ahead and resolved the two threads above since the fixes/tests covered them. tell me if you want another look at anything ? |
28882c5
📝 Changelog fragment recordedThanks! This PR added the changelog fragment(s) below. Pending fragments are folded into |
5 participants
Description
Adds an additive
JobOrderFnplugin that activates only when the existingpriorityplugin's comparison ties (composition is additive persession_plugins.go; no changes topriority.goor the compositionmechanism itself).
Configurable via
modeargument:prefer-larger(default) orprefer-smaller, comparingGetAliveTasksRequestedGPUs()betweensame-priority jobs. Unrecognized
modevalues log a warning and fall backto
prefer-larger. Falls through unchanged to the existingCreationTimestamp/UID tiebreak if GPU counts also match.
A
dominant-resourcemode (comparing by whichever resource represents ajob's largest relative share) was discussed in #1966 but isn't included —
ClusterInfohas no pre-computed total cluster capacity, so it would needits own summation over
Nodesmirroring whatproportion.godoes forqueue fair-share, rather than being a quick addition. Happy to take that
on as a follow-up if there's interest.
Related Issues
Fixes #1966
Checklist
deferral, both modes' comparison behavior, same-priority-same-GPU
fallthrough, unrecognized-mode fallback
make changelogBreaking Changes
None — purely additive; no existing plugin or behavior is modified.
Additional Notes
Full design discussion and the live reversed-creation-order test that
motivated this are in #1966.